fix: address MEDIUM review findings (light witness resilience, proxy CORS, threshold warning) - #1382
fix: address MEDIUM review findings (light witness resilience, proxy CORS, threshold warning)#1382lklimek wants to merge 4 commits into
Conversation
…he client Previously any single witness returning a first/new header that disagreed with the primary would abort the light client with ErrConflictingWitnessHeader, letting one faulty or partitioned witness break liveness for the whole client. Now a single conflicting witness (with at least one corroborating witness) is removed from the trusted set, while two or more conflicting witnesses, or a conflict with no corroboration, still fail closed. This prevents one bad witness from taking down the client while preserving safety against genuine forks. Co-Authored-By: Claudius the Magnificent <noreply@anthropic.com>
…trust)
The light proxy previously accepted WebSocket connections from any origin
(func(*http.Request) bool { return true }), allowing cross-site clients to
subscribe to consensus/block streams.
The proxy now uses rpcserver.OriginChecker against the configured
rpc.cors-allowed-origins list, defaulting to an empty list (reject all) for a
zero-trust posture. Operators opt in by setting cors-allowed-origins in config.
Co-Authored-By: Claudius the Magnificent <noreply@anthropic.com>
…2/3+1 When validators.BelowStrictThreshold() is true the node can no longer guarantee safety, yet previously continued silently. The state machine now logs an explicit "OVERRIDE ENABLED - NOT SAFE FOR PRODUCTION" error so operators are aware they are running an unsafe override. Also documents the behavior change and that cors-allowed-origins governs the light proxy. Co-Authored-By: Claudius the Magnificent <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR addresses three MEDIUM-severity review findings by improving light client resilience to faulty witnesses, tightening the light proxy’s WebSocket origin policy, and making unsafe validator-threshold overrides explicitly visible to operators.
Changes:
- Light client: tolerate a single conflicting witness when the primary is corroborated (remove the dissenting witness instead of failing).
- Light proxy: switch WebSocket origin checks from allow-all to
rpcserver.OriginCheckerdriven by configured allowed origins. - Consensus: emit a prominent log when the validator voting threshold is below the strict 2/3+1 safety floor, plus document behavior changes.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| light/proxy/proxy.go | Enforces a stricter default WebSocket origin policy using rpcserver.OriginChecker and a new AllowedOrigins field. |
| cmd/tenderdash/commands/light.go | Wires rpc.cors-allowed-origins into the light proxy’s WebSocket origin allow-list. |
| light/client.go | Updates witness cross-check logic to remove a single dissenting witness when corroboration exists; fail closed on stronger fork evidence. |
| light/detector_internal_test.go | Expands test cases to reflect new “single dissenting witness removed” and “multiple dissenting witnesses fail” behavior. |
| light/client_test.go | Updates client behavior test to expect removal (not failure) for a single conflicting witness with corroboration. |
| internal/consensus/state_data.go | Promotes below-threshold condition to a prominent log message for operator visibility. |
| docs/nodes/configuration.md | Documents that the RPC CORS origins list also governs light proxy WebSocket origin policy. |
| CHANGELOG.md | Records the behavioral changes (proxy origin policy, witness handling, below-threshold warning). |
|
⛔ Blockers found — Sonnet deferred (commit f4507f5) |
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The light-client witness change introduces two blocking defects: it weakens the documented any-honest-witness security model and can panic when bad and conflicting witnesses appear together. The WebSocket policy remains bypassable through DNS rebinding, and the validator-threshold warning can both miss unsafe transitions and falsely claim that a canonical threshold is an override. These issues require changes before merge.
Source: reviewers codex/general=gpt-5.6-sol(completed); codex/tenderdash-consensus-security=gpt-5.6-sol(completed); verifier=codex/verifier=gpt-5.6-sol(completed); coordinator=openclaw-agent/cliproxy/gpt-5.6-sol(orchestration-only).
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— tenderdash-consensus-security (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 3 blocking | 🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `light/client.go`:
- [BLOCKING] light/client.go:888-912: A colluding primary and witness can evict the sole honest witness
This changes the light client from the documented any-honest-witness model to an unenforced provider-majority model. If a malicious primary serves a validly signed non-canonical fork, one colluding witness returns the same header, and the sole honest witness returns the canonical header, the counters are one corroborating and one conflicting witness. The code removes the honest witness and then persists the primary's fork. Witness comparison exists specifically to detect an otherwise valid conflicting fork after validator trust assumptions have failed, and `docs/tendermint-core/light-client.md` currently says the client is unsafe only when all witnesses are malicious. Fail closed when both sides provide conflicting valid headers, or explicitly adopt, document, and enforce a suitable honest-majority provider model rather than automatically selecting the primary's side.
- [BLOCKING] light/client.go:883-909: Sequential removals use stale witness indices and can panic
`conflictingWitnesses` stores indices from the original witness slice, but `removeWitnesses(witnessesToRemove)` first reorders and truncates that slice using swap-with-last. With witnesses `[matching, bad, conflicting]`, removing bad index 1 moves the conflict to index 1 and leaves a two-element slice; the subsequent log evaluates `c.witnesses[2]` and panics. Other orderings can remove the wrong provider instead. Do not mutate `c.witnesses` until every provider selected for removal has been captured; if dissent removal remains after resolving the security issue above, combine all indices into one removal operation and add a regression test containing matching, bad, and conflicting witnesses together.
In `light/proxy/proxy.go`:
- [BLOCKING] light/proxy/proxy.go:123: Same-host fallback permits DNS-rebinding WebSocket access
`OriginChecker` trusts the request's unvalidated `Host` header whenever it matches the Origin host, even when the configured allow-list is empty. An attacker can serve a page from `http://evil.example:8888`, rebind that hostname to the victim's loopback or private address, and connect to `ws://evil.example:8888/websocket`. The browser then sends both `Origin: http://evil.example:8888` and `Host: evil.example:8888`, so the check succeeds against the default localhost light proxy. This exposes WebSocket RPC methods, including transaction broadcasting and subscriptions, despite the claimed zero-trust default. Reject Origin-bearing requests unless the origin is explicitly configured, or independently validate `Host` against trusted listener or reverse-proxy hostnames.
In `internal/consensus/state_data.go`:
- [SUGGESTION] internal/consensus/state_data.go:269-280: Unsafe-threshold warning can be skipped and mislabels canonical quorums
The outer condition does not account for changes to quorum type or total validator power, although both affect `BelowStrictThreshold()`. For example, a same-quorum set with an unchanged explicit threshold of 800 can grow from 11 validators to 12: the strict floor rises from 734 to 801, making the configuration unsafe, but the quorum hash and raw threshold remain unchanged and no error is logged. The message is also factually wrong for canonical thresholds: `DEVNET_PLATFORM` with 12 validators derives an 800 threshold while `VotingPowerThreshold == 0`, so it is below the 801 strict floor without any override. Trigger the check whenever the effective safety inputs change and add the `OVERRIDE ENABLED` prefix only for an explicit override.
| // A single conflicting witness must not be allowed to break verification: a | ||
| // lone dissenter is far more likely to be a faulty/compromised witness than a | ||
| // sign that the primary is malicious, and failing here would let one bad | ||
| // witness denial-of-service an otherwise-healthy light client. We therefore | ||
| // treat a single conflicting witness as faulty and remove it, exactly like | ||
| // errBadWitness. We still FAIL (preserving the safety-over-availability | ||
| // guarantee) only when the conflict is corroborated: | ||
| // - two or more witnesses disagree with the primary (genuine fork / primary | ||
| // likely faulty), OR | ||
| // - at least one witness conflicts and NO witness corroborates the primary | ||
| // (we cannot establish trust in either party, so we refuse to proceed). | ||
| if len(conflictingWitnesses) > 1 || (len(conflictingWitnesses) == 1 && corroborating == 0) { | ||
| return fmt.Errorf("%w: witnesses %v", ErrConflictingWitnessHeader, conflictingWitnesses) | ||
| } | ||
|
|
||
| // Exactly one conflicting witness with at least one corroborating witness: | ||
| // drop the dissenter and continue. | ||
| if len(conflictingWitnesses) == 1 { | ||
| c.logger.Warn("single witness conflicts with the primary but others corroborate it; "+ | ||
| "removing the dissenting witness rather than failing verification", | ||
| "witness", c.witnesses[conflictingWitnesses[0]]) | ||
| if err := c.removeWitnesses(conflictingWitnesses); err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: A colluding primary and witness can evict the sole honest witness
This changes the light client from the documented any-honest-witness model to an unenforced provider-majority model. If a malicious primary serves a validly signed non-canonical fork, one colluding witness returns the same header, and the sole honest witness returns the canonical header, the counters are one corroborating and one conflicting witness. The code removes the honest witness and then persists the primary's fork. Witness comparison exists specifically to detect an otherwise valid conflicting fork after validator trust assumptions have failed, and docs/tendermint-core/light-client.md currently says the client is unsafe only when all witnesses are malicious. Fail closed when both sides provide conflicting valid headers, or explicitly adopt, document, and enforce a suitable honest-majority provider model rather than automatically selecting the primary's side.
source: ['codex']
| return err | ||
| } | ||
|
|
||
| // A witness presenting a header that conflicts with the primary's means the | ||
| // primary's header cannot be trusted: fail the comparison. | ||
| if len(conflictingWitnesses) > 0 { | ||
| // A single conflicting witness must not be allowed to break verification: a | ||
| // lone dissenter is far more likely to be a faulty/compromised witness than a | ||
| // sign that the primary is malicious, and failing here would let one bad | ||
| // witness denial-of-service an otherwise-healthy light client. We therefore | ||
| // treat a single conflicting witness as faulty and remove it, exactly like | ||
| // errBadWitness. We still FAIL (preserving the safety-over-availability | ||
| // guarantee) only when the conflict is corroborated: | ||
| // - two or more witnesses disagree with the primary (genuine fork / primary | ||
| // likely faulty), OR | ||
| // - at least one witness conflicts and NO witness corroborates the primary | ||
| // (we cannot establish trust in either party, so we refuse to proceed). | ||
| if len(conflictingWitnesses) > 1 || (len(conflictingWitnesses) == 1 && corroborating == 0) { | ||
| return fmt.Errorf("%w: witnesses %v", ErrConflictingWitnessHeader, conflictingWitnesses) | ||
| } | ||
|
|
||
| // Exactly one conflicting witness with at least one corroborating witness: | ||
| // drop the dissenter and continue. | ||
| if len(conflictingWitnesses) == 1 { | ||
| c.logger.Warn("single witness conflicts with the primary but others corroborate it; "+ | ||
| "removing the dissenting witness rather than failing verification", | ||
| "witness", c.witnesses[conflictingWitnesses[0]]) | ||
| if err := c.removeWitnesses(conflictingWitnesses); err != nil { |
There was a problem hiding this comment.
🔴 Blocking: Sequential removals use stale witness indices and can panic
conflictingWitnesses stores indices from the original witness slice, but removeWitnesses(witnessesToRemove) first reorders and truncates that slice using swap-with-last. With witnesses [matching, bad, conflicting], removing bad index 1 moves the conflict to index 1 and leaves a two-element slice; the subsequent log evaluates c.witnesses[2] and panics. Other orderings can remove the wrong provider instead. Do not mutate c.witnesses until every provider selected for removal has been captured; if dissent removal remains after resolving the security issue above, combine all indices into one removal operation and add a regression test containing matching, bad, and conflicting witnesses together.
source: ['codex']
| // permit specific browser origins set Proxy.AllowedOrigins (mirrors | ||
| // rpc.Config.CORSAllowedOrigins); a "*" entry allows any origin but is | ||
| // discouraged. This replaces the previous blanket allow-all policy. | ||
| wm.CheckOrigin = rpcserver.OriginChecker(p.Logger, p.AllowedOrigins) |
There was a problem hiding this comment.
🔴 Blocking: Same-host fallback permits DNS-rebinding WebSocket access
OriginChecker trusts the request's unvalidated Host header whenever it matches the Origin host, even when the configured allow-list is empty. An attacker can serve a page from http://evil.example:8888, rebind that hostname to the victim's loopback or private address, and connect to ws://evil.example:8888/websocket. The browser then sends both Origin: http://evil.example:8888 and Host: evil.example:8888, so the check succeeds against the default localhost light proxy. This exposes WebSocket RPC methods, including transaction broadcasting and subscriptions, despite the claimed zero-trust default. Reject Origin-bearing requests unless the origin is explicitly configured, or independently validate Host against trusted listener or reverse-proxy hostnames.
source: ['codex']
Why this PR exists
ErrConflictingWitnessHeader), breaking liveness for honest users; (2) the light proxy accepts WebSocket connections from any origin, enabling cross-site subscription to consensus streams; (3) running below the 2/3+1 voting-power threshold proceeds silently with no operator warning that safety is no longer guaranteed.v1.6-dev.Issue being fixed or feature implemented
Addresses three review findings:
voting_power_thresholddrops below 2/3+1.What was done?
light/client.go: single conflicting witness (with ≥1 corroborating) is removed from the trusted set; ≥2 conflicting witnesses, or a conflict with zero corroboration, still fail closed.light/proxy/proxy.go: replaced the permissivefunc(*http.Request) bool { return true }origin check withrpcserver.OriginCheckerover a newAllowedOriginsfield (zero-trust default: empty list rejects all).cmd/tenderdash/commands/light.gowiresconf.RPC.CORSAllowedOriginsinto the proxy.internal/consensus/state_data.go: logs an explicitOVERRIDE ENABLED - NOT SAFE FOR PRODUCTIONerror whenvalidators.BelowStrictThreshold()is true.CHANGELOG.md+docs/nodes/configuration.md: document the behavior changes.How Has This Been Tested?
gofmtclean on all changed files.go vet ./light/ ./light/proxy/ ./internal/consensus/clean (BLS native deps built viamake build-bls).go test ./light/passes (13.4s), including updatedTestCompareFirstHeaderWithWitness*cases: single conflicting witness → removed (no error), two conflicting → fail, single conflicting with no corroboration → fail.go build ./light/proxy/ ./cmd/tenderdash/commands/succeeds.Breaking Changes
None in the Go API. Behavioral: the light proxy now rejects WebSocket connections from unconfigured origins by default (previously accepted all). Operators relying on the open behavior must set
rpc.cors-allowed-origins.Checklist:
🤖 Co-authored by Claudius the Magnificent AI Agent